❮

Remove fls(), use pg_bitutils.h facilities instead?

  • Jump to comment-1
    thomas.munro@gmail.com2022-07-20T04:20:46+00:00
    Hi, Back in commit 4f658dc8 we gained src/port/fls.c. As anticipated by its commit message, we later finished up with something better in src/include/port/pg_bitutils.h. fls() ("find last set") is an off-by-one cousin of pg_leftmost_one_pos32(). I don't know why ffs() ("find first set", the rightmost variant) made it into POSIX while fls() did not, other than perhaps its more amusing name. fls() is present on *BSD, Macs and maybe more, but not everywhere, hence the configure test. Let's just do it with pg_bitutils.h instead, and drop some cruft? Open to better ideas on whether we need a new function, or there is some way to use the existing facilities directly without worrying about undefined behaviour for 0, etc. Noticed while looking for configure stuff to cull. Mentioning separately because this isn't a simple no-longer-needed-crutch-for-prestandard-system case like the others in a nearby thread.
    • Jump to comment-1
      tgl@sss.pgh.pa.us2022-07-20T04:52:54+00:00
      Thomas Munro <thomas.munro@gmail.com> writes: > Back in commit 4f658dc8 we gained src/port/fls.c. As anticipated by > its commit message, we later finished up with something better in > src/include/port/pg_bitutils.h. fls() ("find last set") is an > off-by-one cousin of pg_leftmost_one_pos32(). I don't know why ffs() > ("find first set", the rightmost variant) made it into POSIX while > fls() did not, other than perhaps its more amusing name. fls() is > present on *BSD, Macs and maybe more, but not everywhere, hence the > configure test. Let's just do it with pg_bitutils.h instead, and drop > some cruft? Open to better ideas on whether we need a new function, I think we could probably just drop fls() entirely. It doesn't look to me like any of the existing callers expect a zero argument, so they could be converted to use pg_leftmost_one_pos32() pretty trivially. I don't see that fls() is buying us anything that is worth requiring readers to know yet another nonstandard function. regards, tom lane
      • Jump to comment-1
        thomas.munro@gmail.com2022-07-20T05:26:10+00:00
        On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think we could probably just drop fls() entirely. It doesn't look > to me like any of the existing callers expect a zero argument, so they > could be converted to use pg_leftmost_one_pos32() pretty trivially. > I don't see that fls() is buying us anything that is worth requiring > readers to know yet another nonstandard function. That was not true for the case in contiguous_pages_to_segment_bin(), in dsa.c. If it's just one place like that (and, hrrm, curiously there is an open issue about binning quality on my to do list...), then perhaps we should just open code it there. The attached doesn't trigger the assertion that work != 0 in a simple make check.
        • Jump to comment-1
          tgl@sss.pgh.pa.us2022-07-20T13:34:37+00:00
          Thomas Munro <thomas.munro@gmail.com> writes: > On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> I think we could probably just drop fls() entirely. It doesn't look >> to me like any of the existing callers expect a zero argument, so they >> could be converted to use pg_leftmost_one_pos32() pretty trivially. > That was not true for the case in contiguous_pages_to_segment_bin(), > in dsa.c. If it's just one place like that (and, hrrm, curiously > there is an open issue about binning quality on my to do list...), How is it sane to ask for a segment bin for zero pages? Seems like something should have short-circuited such a case well before here. regards, tom lane
        • Jump to comment-1
          thomas.munro@gmail.com2022-07-20T05:44:35+00:00
          On Wed, Jul 20, 2022 at 5:26 PM Thomas Munro <thomas.munro@gmail.com> wrote: > On Wed, Jul 20, 2022 at 4:52 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I think we could probably just drop fls() entirely. It doesn't look > > to me like any of the existing callers expect a zero argument, so they > > could be converted to use pg_leftmost_one_pos32() pretty trivially. > > I don't see that fls() is buying us anything that is worth requiring > > readers to know yet another nonstandard function. > > That was not true for the case in contiguous_pages_to_segment_bin(), > in dsa.c. If it's just one place like that (and, hrrm, curiously > there is an open issue about binning quality on my to do list...), > then perhaps we should just open code it there. The attached doesn't > trigger the assertion that work != 0 in a simple make check. That double eval macro wasn't nice. This time with a static inline function.
          • Jump to comment-1
            tgl@sss.pgh.pa.us2022-07-20T13:48:36+00:00
            Thomas Munro <thomas.munro@gmail.com> writes: > That double eval macro wasn't nice. This time with a static inline function. Seems like passing a size_t to pg_leftmost_one_pos32 isn't great. It was just as wrong before (if the caller-supplied argument is indeed a size_t), but no time like the present to fix it. We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t as the appropriate one of pg_leftmost_one_pos32/64, perhaps. regards, tom lane
            • Jump to comment-1
              thomas.munro@gmail.com2022-07-21T08:13:52+00:00
              On Thu, Jul 21, 2022 at 1:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > How is it sane to ask for a segment bin for zero pages? Seems like > something should have short-circuited such a case well before here. It's intended. There are two ways you can arrive here with n == 0: * There's a special case in execParallel.c that creates a DSA segment "in-place" with initial size dsa_minimum_size(). That's because we don't know yet if we have any executor nodes that need a DSA segment (Parallel Hash, Parallel Bitmap Heap Scan), so we create one with the minimum amount of space other than the DSA control meta-data, so you get an in-place segment 0 with 0 usable pages. As soon as someone tries to allocate one byte, the first external DSM segment will be created. * A full segment can be re-binned into slot 0. On Thu, Jul 21, 2022 at 1:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Seems like passing a size_t to pg_leftmost_one_pos32 isn't great. > It was just as wrong before (if the caller-supplied argument is > indeed a size_t), but no time like the present to fix it. > > We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t > as the appropriate one of pg_leftmost_one_pos32/64, perhaps. Yeah.
              • Jump to comment-1
                tgl@sss.pgh.pa.us2022-07-21T13:46:05+00:00
                Thomas Munro <thomas.munro@gmail.com> writes: > On Thu, Jul 21, 2022 at 1:34 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: >> How is it sane to ask for a segment bin for zero pages? Seems like >> something should have short-circuited such a case well before here. > It's intended. There are two ways you can arrive here with n == 0: OK. >> We could have pg_bitutils.h #define pg_leftmost_one_pos_size_t >> as the appropriate one of pg_leftmost_one_pos32/64, perhaps. > Yeah. Patches look good to me. regards, tom lane
    • Jump to comment-1
      dgrowleyml@gmail.com2022-07-20T04:43:03+00:00
      On Wed, 20 Jul 2022 at 16:21, Thomas Munro <thomas.munro@gmail.com> wrote: > Back in commit 4f658dc8 we gained src/port/fls.c. As anticipated by > its commit message, we later finished up with something better in > src/include/port/pg_bitutils.h. fls() ("find last set") is an > off-by-one cousin of pg_leftmost_one_pos32(). Seems like a good idea to me. One thing I noticed was that pg_leftmost_one_pos32() expects a uint32 but the new function passes it an int. Is it worth mentioning that's ok in a comment somewhere or maybe adding an explicit cast? David